Skip to content

[TrimmableTypeMap] Runtime and typemap generator fixes#11145

Open
simonrozsival wants to merge 16 commits intomainfrom
dev/simonrozsival/trimmable-runtime-fixes
Open

[TrimmableTypeMap] Runtime and typemap generator fixes#11145
simonrozsival wants to merge 16 commits intomainfrom
dev/simonrozsival/trimmable-runtime-fixes

Conversation

@simonrozsival
Copy link
Copy Markdown
Member

@simonrozsival simonrozsival commented Apr 17, 2026

Part 4 of the split of the trimmable-test-plumbing branch. (PR-1: #11142, PR-2: #11144, PR-3: #11143)

A collection of correctness fixes for the trimmable typemap runtime and code
generator, uncovered while stabilizing the new CoreCLRTrimmable test lane.

Runtime / managed side

  • Fix runtime initialization ordering and generic type crash — a generic proxy type was being resolved before the typemap was ready, causing a NullReferenceException on startup.
  • Fix generic type proxy loading in the typemap — the generic definition's Type.FullName includes a mangled form that did not match the proxy's lookup key.
  • Revert two-phase init — use a single Initialize () after runtime creation. The two-phase scheme was only needed for a register-natives workaround that no longer applies.
  • Fix ClassLoader mismatch in RegisterNatives — use the class reference passed from Java (nativeClassHandle) via new JniType (ref classRef, Copy) instead of new JniType (className), because FindClass may resolve to a different class from a different ClassLoader.
  • Fix Release build pipeline and ApplicationRegistration — the ApplicationRegistration helper was not being emitted in Release where the generator ran under a different code path.

Code generator

  • Include abstract Instrumentation/Application subtypes in ApplicationRegistration. Their native methods (n_OnCreate, n_OnStart, etc.) are declared on the abstract base class and must be registered via ApplicationRegistration.registerApplications ().
  • Self-apply generated proxy types as their own [JavaPeerProxy] custom attribute so the runtime can instantiate them via type.GetCustomAttribute<JavaPeerProxy> () for AOT-safe resolution.

Closed-generic peer lookup (follow-up commits)

Closed generic instantiations (e.g. JavaList<string>) were silently missing from the typemap: the generator correctly emits entries keyed by the open generic definition (JavaList<>), but the runtime only did an identity TryGetValue, so closed instantiations returned null.

  • GTD fallback in GetProxyForManagedType — after a direct miss, retry the dictionary with type.GetGenericTypeDefinition () when the request is a closed generic. GetGenericTypeDefinition is AOT- and trim-safe (it is not annotated RequiresDynamicCode/RequiresUnreferencedCode, unlike MakeGenericType). The resolved proxy is cached under the original closed type.
  • Open-generic aware TargetTypeMatches helper — the hierarchy lookup's IsAssignableFrom check is supplemented by a base-chain walk (closed subclass of an open generic peer, e.g. class MyList : JavaList<int>) and a GetInterfaces () walk (closed class implementing an open generic interface peer). IL2070 is suppressed with a justification: targetType originates at JNI marshalling call sites where the caller's generic signature keeps its base chain and interface set rooted.
  • Scope is deliberately managed→JNI onlyJavaPeerProxy.CreateInstance on an open-generic proxy still throws NotSupportedException: we never construct closed generic peers from Java, and MakeGenericType is not safe under full NativeAOT anyway.

Tests

  • Generate_ProxyType_IsSelfAppliedAsCustomAttribute — invariant has regressed twice; lock it down.
  • Execute_CollectsDeferredRegistrationTypes — reflects that abstract Instrumentation/Application subtypes are now included.
  • TrimmableTypeMapTypeManagerTests (device suite) — 12 new tests for the closed-generic lookup path: managed→JNI resolution via GTD (direct/subclass/unknown/cached/non-generic), pure TargetTypeMatches assertions covering direct assignability, self-match, closed subclass, mismatched arity, interface implementer, inherited interface, and unrelated-hint negatives.

Known gap / follow-up

Descendants of Application and Instrumentation that are themselves concrete can still fail to register their native methods when the base is abstract and the subclass is loaded before the managed runtime is ready. The current PR intentionally does not land a 'propagate CannotRegisterInStaticConstructor to descendants' style fix.

Instead, I will open a follow-up issue proposing to unify the registration approach — always defer registration via __md_registerNatives () for all types — and drop the propagation machinery entirely, with a possible future optimization to use static { registerNatives } for provably safe types.

Scope

Host unit tests: 355/355 pass locally; device tests run in CI (CoreCLRTrimmable lane).

Copilot AI review requested due to automatic review settings April 17, 2026 10:21
@simonrozsival simonrozsival added copilot `copilot-cli` or other AIs were used to author this trimmable-type-map labels Apr 17, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR delivers a set of correctness fixes for the trimmable typemap path, spanning runtime initialization/native registration behavior, typemap/proxy emission, and regression tests to prevent repeat breakages discovered while stabilizing the CoreCLRTrimmable test lane.

Changes:

  • Adjust typemap generator output to handle open generic proxy scenarios and ensure generated proxy types are self-applied as [JavaPeerProxy] attributes.
  • Fix runtime native registration to use the nativeClassHandle class reference passed from Java to avoid ClassLoader mismatches.
  • Update and add generator regression tests to cover self-applied proxy attributes and expanded deferred registration type collection.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs Adds regression test to ensure generated proxies carry self-applied custom attributes.
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TrimmableTypeMapGeneratorTests.cs Updates deferred-registration expectations to include abstract Application/Instrumentation subtypes.
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.TypeMap.Trimmable.targets Changes _RemoveRegisterAttribute override to repoint _Shrunk* item groups back to _Resolved*.
src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs Updates native registration to avoid FindClass/ClassLoader mismatches when registering natives.
src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs Broadens deferred-registration type collection to include types that still generate ACWs (including abstract subtypes).
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs Emits proxies with Java.Lang.Object as the base generic arg for open generic definitions; adds self-applied proxy attribute emission.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ModelBuilder.cs Skips emitting TypeMapAssociation entries for generic definitions.

Comment thread src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs Outdated
Comment thread src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ModelBuilder.cs Outdated
Comment thread src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs Outdated
Comment thread src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs Outdated
Comment thread src/Xamarin.Android.Build.Tasks/Tasks/ProcessAssemblies.cs Outdated
simonrozsival and others added 15 commits April 18, 2026 02:21
Part 4 of the split of the trimmable-test-plumbing branch. A collection of
correctness fixes for the trimmable typemap runtime and code generator,
uncovered while stabilizing the new CoreCLRTrimmable test lane.

### Runtime / managed side

 * Fix runtime initialization ordering and generic type crash — a generic
   proxy type was being resolved before the typemap was ready, causing a
   NullReferenceException on startup.
 * Fix generic type proxy loading in the typemap — the generic definition's
   Type.FullName includes a mangled form that did not match the proxy's
   lookup key.
 * Revert two-phase init — use a single Initialize () after runtime
   creation. The two-phase scheme was only needed for a register-natives
   workaround that no longer applies.
 * Fix ClassLoader mismatch in RegisterNatives — use the class reference
   passed from Java (nativeClassHandle) via JniType (ref classRef, Copy)
   instead of resolving the name with JniType (string), because FindClass
   may resolve to a different class from a different ClassLoader.
 * Fix Release build pipeline and ApplicationRegistration — the
   ApplicationRegistration helper was not being emitted in Release where
   the generator ran under a different code path.

### Code generator

 * Include abstract Instrumentation/Application subtypes in
   ApplicationRegistration. Their native methods (n_OnCreate, n_OnStart,
   etc.) are declared on the abstract base class and must be registered
   via ApplicationRegistration.registerApplications ().
 * Self-apply generated proxy types as their own [JavaPeerProxy] custom
   attribute so the runtime can instantiate them via
   type.GetCustomAttribute<JavaPeerProxy> () for AOT-safe resolution.

### Tests

 * New regression test Generate_ProxyType_IsSelfAppliedAsCustomAttribute
   asserts every generated proxy type carries its own self-applied custom
   attribute. This invariant has regressed twice; lock it down.
 * Update Execute_CollectsDeferredRegistrationTypes to reflect that
   abstract Instrumentation/Application subtypes are now included in
   ApplicationRegistrationTypes.

### Known gap / follow-up

Descendants of Application and Instrumentation that are themselves concrete
still fail to register their native methods in the lazy
__md_registerNatives pattern when the base is abstract and the subclass is
loaded before the managed runtime is ready. The current PR intentionally
does not land the 'propagate CannotRegisterInStaticConstructor to
descendants' fix — instead we plan to open a follow-up issue to unify the
registration approach (always defer via __md_registerNatives) and drop the
propagation machinery entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- C3: keep the `JniType(string)` overload but rewrite the comment to
  explain exactly why — its `TryFindClass(string, bool)` path falls back
  to `Class.forName(name, true, info.Runtime.ClassLoader)`, which
  resolves via the runtime's app ClassLoader (the one that loaded
  `mono.android.Runtime` from the APK). The `ReadOnlySpan<byte>`
  overload only calls raw `FindClass` and would resolve via the
  system ClassLoader, returning a different `Class` instance.
- C7: restore `OnRegisterNatives` visibility from `internal static`
  back to `static` (private). There are no in-repo callers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- C2: emit generic-definition proxies deriving from the non-generic
  `JavaPeerProxy` base (3-arg ctor: `string jniName, Type targetType,
  Type? invokerType`) instead of `JavaPeerProxy<Java.Lang.Object>`.
  Concrete-target proxies still use `JavaPeerProxy<T>` (2-arg ctor).
  `EmitTypeReferences` was renamed accordingly and the `.ctor` body
  now pushes `targetTypeRef` via `ldtoken`/`GetTypeFromHandle` when
  emitting against the non-generic base.
  The skip was defensive because the old generic proxy emit failed at
  load; with C2 in place the proxy loads cleanly, so generic
  definitions can get `[TypeMapAssociation]` entries again. Added a
  regression test (`Fixture_GenericHolder_HasAssociation`).
- C8: relax `Generate_ProxyType_IsSelfAppliedAsCustomAttribute` to
  accept both `MemberReference` (parent is the proxy `TypeDefinition`)
  and `MethodDefinition` whose declaring type is the proxy. Also relax
  `Generate_ProxyType_UsesGenericJavaPeerProxyBase` to allow
  `HandleKind.TypeReference` for open-generic proxies now that they
  inherit from the non-generic base.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Investigated the suggestion to drop the lazy `__md_registerNatives()`
emission for `CannotRegisterInStaticConstructor` types (Application +
Instrumentation subtypes). Conclusion: we keep it, and the comment has
been updated to explain the ordering constraint that makes it
necessary.

`ApplicationRegistration.registerApplications()` is invoked from
`MonoPackageManager.LoadApplication`, which runs from
`MonoRuntimeProvider.attachInfo`. But `Application.attachBaseContext`
fires *before* ContentProvider.attachInfo — so a user Application that
overrides `attachBaseContext` can trigger a native callback before
`registerApplications()` has run. The lazy one-time helper in each
native-method wrapper is the safety net that covers that window.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…trimmable typemap path

The legacy `_RemoveRegisterAttribute` target (in Xamarin.Android.Common.targets)
is a no-op copy step that mirrors `@(_ResolvedAssemblies)` into
`@(_ShrunkAssemblies)` at `<dir>/shrunk/<file>` paths. `ProcessAssemblies`
unconditionally rewrote `ShrunkAssemblies` to those `shrunk/` paths
whenever `PublishTrimmed && !AndroidIncludeDebugSymbols` was true.

The trimmable typemap path does not strip `[Register]` attributes and
does not need the `shrunk/` copy; the previous code papered over the
mismatch by overriding `_RemoveRegisterAttribute` to rebuild
`@(_ShrunkAssemblies)` from `@(_ResolvedAssemblies)` at that stage —
fragile and hard to follow (flagged by Copilot reviewer as C6 because
the override also dropped the `DependsOnTargets="_PrepareAssemblies"`
and the `AndroidLinkMode != 'None'` conditions from the base target).

Fix:

* Thread the `_AndroidTypeMapImplementation` property into
  `<ProcessAssemblies>` via a new `AndroidTypeMapImplementation`
  string input.
* Skip the `shrunk/` path rewrite when the input is "trimmable", so
  `ShrunkAssemblies` is simply `OutputAssemblies` for that path.
* Reduce the trimmable override of `_RemoveRegisterAttribute` to an
  empty target — the base target's copy is no longer needed because
  `@(_ShrunkAssemblies) == @(_ResolvedAssemblies)` already.

C6 is resolved as a side effect: there is no longer any target body
that could depend on `_PrepareAssemblies` or the linker conditions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The typemap generator emits one TypeMapAssociation per generic peer, keyed
by the open generic definition (e.g. typeof(JavaList<>)). The CLR
TypeMapping dictionary does identity-based lookups (see dotnet/runtime's
TypeMapLazyDictionary.cs), so closed instantiations such as
typeof(JavaList<string>) never match the registered open entry directly.
Previously GetProxyForManagedType did a single TryGetValue on that
dictionary and silently returned null for every closed generic peer,
breaking managed→JNI name resolution for things like JavaList<string>.

- GetProxyForManagedType: after a direct miss, if the type is a closed
  generic, retry with type.GetGenericTypeDefinition(). GetGenericTypeDefinition
  is safe under AOT + full trim (it is not RequiresDynamicCode). The
  resolved proxy is cached under the original closed Type so subsequent
  lookups remain O(1).
- TryGetProxyFromHierarchy: the per-proxy TargetType for generic peers is
  the open generic definition, so targetType.IsAssignableFrom(proxy.TargetType)
  returned false when the caller hint was a closed instantiation. Extract
  a TargetTypeMatches helper that additionally accepts a hint whose base
  chain contains a closed form of the proxy's open generic definition —
  no type-argument comparison since Java erases generics and one proxy
  serves every closed instantiation.

Adds a device-side test that exercises the GTD fallback through the public
TryGetJniNameForManagedType path using JavaList<T>.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit added a single device test exercising the GTD
fallback via JavaList<T>, but that scenario only hits the direct
IsAssignableFrom branch of TargetTypeMatches (JavaList<T> erases to the
non-generic JavaList, whose proxy's TargetType is the non-generic type).
The open-generic branches of TargetTypeMatches were not exercised.

- Promote TargetTypeMatches from a local static inside GetProxyForJavaObject
  to an internal static member of TrimmableTypeMap so it can be unit-tested
  directly.
- Add pure-function tests for TargetTypeMatches covering: direct assignable,
  open generic self-match, closed subclass base-walk, deeply closed
  subclass, different open GTDs, unrelated types.
- Add regression tests for GetProxyForManagedType:
    * non-generic type still resolves directly (regression shield for the
      fast path)
    * an unknown closed generic (e.g. List<int>) misses both the direct
      and GTD lookups and returns false
    * repeated lookups of the same closed generic are idempotent, verifying
      the cache is keyed by the original closed type.

All new tests are gated on the RuntimeFeature.TrimmableTypeMap feature
switch so they are a no-op in non-trimmable configurations.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a generic Android binding is declared as an interface (e.g. an open
generic peer interface like IJavaCollection<T>), the proxy's TargetType is
the open interface definition. The caller's hint in GetProxyForJavaObject
is usually a closed class that implements that interface (from the
marshaller's target signature). The previous base-chain-only walk missed
this case, so the hierarchy lookup silently rejected the correct proxy.

Extend TargetTypeMatches to also scan targetType.GetInterfaces() for any
closed generic interface whose definition equals the open proxy target.
GetInterfaces() on a class also includes interfaces from its base types,
so closed subclasses of types implementing an open generic interface are
covered.

Suppress IL2070 on the helper with a justification: targetType originates
at JNI marshalling call sites where the caller's generic signature roots
the interface set. Legacy Android binding generators have always relied
on this reachability contract.

Adds unit-test coverage for:
- closed class implementing an open generic interface
- closed subclass inheriting the interface implementation
and a standalone C# program confirmed 9/9 assertions pass before committing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous example implied that a hint of `IList<string>` would match a
proxy whose target is `JavaList<>`. It does not: `IList<string>.GetInterfaces()`
returns only its super-interfaces (ICollection<string>, IEnumerable<string>,
IEnumerable), and none of their generic-type-definitions equals `JavaList<>`.

The actual scenario covered by the interface walk is: the proxy target is
an **open generic interface** peer, and the hint is a closed class that
implements a closed instantiation of that interface. For a class hint,
`GetInterfaces()` enumerates every interface the class implements, so the
closed form is discovered and its GTD matches the open interface target.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Covers the case where a closed class hint does not implement any closed
form of the open generic interface that the proxy targets. Also rewrites
the positive-case comment that incorrectly described an `IList<string>`
hint matching a `JavaList<>` proxy (that scenario is not covered by the
interface walk — it would require walking the proxy's own interfaces).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collapse the two-step "try closed, then fall back to GTD" into a single
lookup by normalising the type first. The generator never keys the proxy
typemap on a closed generic instantiation (Java erases generics, so one
open-GTD entry covers every closed form), so the first lookup in the old
code was always a miss for closed generics — remove it.

No behavior change; host tests still 355/355.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The interface walk added to `TargetTypeMatches` (via `Type.GetInterfaces()`)
required an `[UnconditionalSuppressMessage("Trimming", "IL2070", ...)]`
because `GetInterfaces()` is annotated
`[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]`
on its `this` parameter, and propagating that annotation up the call chain
(`TargetTypeMatches` -> `TryGetProxyFromHierarchy` -> `GetProxyForJavaObject`
-> `JavaMarshalValueManager.CreatePeer`) would require modifying Java.Interop's
public `CreatePeer` override signature, which only carries a `Constructors`
DAM today.

Audit of the call path shows the interface walk is dead code in practice:
`TryGetProxyFromHierarchy` walks the JNI class chain only (via
`GetObjectClass` + `GetSuperclass`), never JNI interfaces, so no
open-generic *interface* proxy target ever reaches `TargetTypeMatches`
from this path. The remaining base-chain + GTD-identity logic covers every
case our generator emits.

If a future change needs to resolve proxies through a JNI interface chain,
the right answer is a generator-emitted implementer map (statically trim-safe)
rather than reflecting over `GetInterfaces()` at runtime.

Also drops the two tests and fixture types that exercised the removed
interface walk. Host unit tests: 355/355 pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival simonrozsival force-pushed the dev/simonrozsival/trimmable-runtime-fixes branch from 0a87ba3 to 894f441 Compare April 18, 2026 00:40
@simonrozsival
Copy link
Copy Markdown
Member Author

/review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 18, 2026

Android PR Reviewer completed successfully!

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM

Thorough, well-documented collection of correctness fixes for the trimmable typemap runtime and code generator.

Highlights

  • ClassLoader fix (RegisterNatives / OnRegisterNatives): Switching from ReadOnlySpan<byte>string JniType overload and using JniType(ref classRef, Copy) instead of JniType(className) correctly resolves the app ClassLoader vs. system ClassLoader mismatch. The inline comments explaining why are excellent.
  • Generic type definition fallback: The GetProxyForManagedType GTD normalization is clean — closed generics are mapped to their open definition before cache lookup, matching the generator's keying strategy. TargetTypeMatches base-chain walk is correct and the scope decision (class-only, no interfaces) is well-justified in the XML docs.
  • Self-application pattern: Emitting the proxy type as its own [JavaPeerProxy] custom attribute via a parameterless .ctor that hardcodes the base ctor args is the right AOT-safe approach.
  • !p.DoNotGenerateAcw filter: Including abstract Application/Instrumentation subtypes in deferred registration makes sense since their native methods are declared on the abstract base and must be registered.
  • Test coverage: 12 new device tests covering GTD fallback (direct/subclass/unknown/cached/non-generic) and pure TargetTypeMatches assertions. The Generate_ProxyType_IsSelfAppliedAsCustomAttribute regression guard is valuable given the two prior regressions.

Summary

Severity Count
💡 Suggestion 1

CI: Public checks pass (dotnet-android ✅, license/cla ✅). Internal Xamarin.Android-PR status should be verified separately.

Generated by Android PR Reviewer for issue #11145 · ● 6.6M


// Regression: the GTD fallback must not disturb the non-generic hot path.
Assert.IsTrue (TrimmableTypeMap.Instance.TryGetJniNameForManagedType (typeof (JavaList), out var jniName));
Assert.IsFalse (string.IsNullOrEmpty (jniName));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 💡 Formatting — Repo convention prefers the NullableExtensions instance method over the static call for NRT flow analysis integration:

Suggested change
Assert.IsFalse (string.IsNullOrEmpty (jniName));
Assert.IsFalse (jniName.IsNullOrEmpty ());

Rule: Use IsNullOrEmpty() extension

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

copilot `copilot-cli` or other AIs were used to author this trimmable-type-map

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants